Improve instrument name validation and log messages #6457

hannahhaering · 2025-08-28T15:55:21Z

Changes

I moved the validation of view name and instrument name into Guard helpers. I improved the log message, specifying the incorrect metric name, as suggested in the issue.

The code has changed quite a bit since this issue was opened, so I'm not entirely sure whether my approach matches the original intent.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs

src/Shared/Guard.cs

src/OpenTelemetry/CHANGELOG.md

…to argument exceptions

Co-authored-by: Martin Costello <[email protected]>

src/OpenTelemetry/CHANGELOG.md

src/Shared/Guard.cs

Co-authored-by: Martin Costello <[email protected]>

github-actions · 2025-09-06T03:19:33Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

src/Shared/Guard.cs

Kielek · 2025-09-08T05:17:58Z

src/Shared/Guard.cs

+        // Note: We don't use static readonly here because some customers
+        // replace this using reflection which is not allowed on initonly static
+        // fields. See: https://github.com/dotnet/runtime/issues/11571.
+        // Customers: This is not guaranteed to work forever. We may change this
+        // mechanism in the future do this at your own risk.


It seems to be broking #4136.

In general, it is not against OTel promises, but it might be still useful for @dpk83.

@dpk83, what is your perspective on changes?

@CodeBlanch, FYI as you introduced this functionality.

Probably going to break this: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/f5636e07657c1dfc86d1b04484ade53973eaf25e/src/OpenTelemetry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs#L120-L134

/cc @rajkumar-rangaraj

Should I make the changes in the contrib repo or is there anything else left to do?

Kielek · 2025-09-10T04:53:31Z

src/Shared/Guard.cs

-    internal static class Guard
+    internal static partial class Guard
    {
+        // Note: We don't use static readonly here because some customers


One more thing, for now, new logic seems to be related only to metrics. Maybe you could put all this stuff to separate class under src/OpenTelemetry/Metrics/MetricsGuard.cs (better name more than weslcomed)?

Do you think that we need some new unit tests?

I'm currently working on this.

On the other hand, this doesn't need to be a regular expression. Length check and IndexOfAny would do the job.

…lidation-and-log-messages

codecov · 2025-09-15T02:40:55Z

Codecov Report

❌ Patch coverage is 38.88889% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.64%. Comparing base (5f66728) to head (ed6a109).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rc/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs	8.33%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6457   +/-   ##
=======================================
  Coverage   86.63%   86.64%           
=======================================
  Files         258      259    +1     
  Lines       11888    11887    -1     
=======================================
  Hits        10299    10299           
+ Misses       1589     1588    -1

Flag	Coverage Δ
unittests-Project-Experimental	`86.59% <38.88%> (+0.25%)`	⬆️
unittests-Project-Stable	`86.53% <38.88%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../Metrics/Builder/MeterProviderBuilderExtensions.cs	`98.54% <100.00%> (-0.02%)`	⬇️
...lemetry/Metrics/Builder/MeterProviderBuilderSdk.cs	`100.00% <ø> (+1.63%)`	⬆️
src/OpenTelemetry/Metrics/MeterProviderSdk.cs	`93.36% <100.00%> (ø)`
src/OpenTelemetry/Metrics/MetricGuard.cs	`100.00% <100.00%> (ø)`
...elemetry/Metrics/View/MetricStreamConfiguration.cs	`100.00% <100.00%> (ø)`
...rc/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs	`87.28% <8.33%> (-4.68%)`	⬇️

... and 2 files with indirect coverage changes

martincostello · 2025-09-18T09:12:25Z

src/OpenTelemetry/CHANGELOG.md

+* Moved the `InstrumentNameRegex` from `MeterProviderBuilderSdk` to `Guard`.
+  ([#6457](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6457))
+


This sounds like an internal implementation detail - all users need to know is what the external-facing behaviour change is.

I agree. I added it to the changelog because the regex is public and someone may change it using reflection:
#6457 (comment)

I can also omit this in the changelog, if it is not necessary.

Ah OK - I didn't realise it was public.

It wasn't . It was known internal feature used by reflection. Mostly by MS teams.

github-actions · 2025-09-27T03:19:44Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

…essages

Improve instrument name validation and log messages

454f7c8

github-actions bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Aug 28, 2025

hannahhaering marked this pull request as ready for review August 28, 2025 15:55

hannahhaering requested a review from a team as a code owner August 28, 2025 15:55

martincostello reviewed Aug 28, 2025

View reviewed changes

src/OpenTelemetry/Metrics/Reader/MetricReaderExt.cs Show resolved Hide resolved

src/Shared/Guard.cs Outdated Show resolved Hide resolved

src/Shared/Guard.cs Outdated Show resolved Hide resolved

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

hannahhaering and others added 2 commits August 28, 2025 23:10

use RegEx source generator for net7_0_or_greater; add parameter name …

d61dc99

…to argument exceptions

Update src/OpenTelemetry/CHANGELOG.md

210bfed

Co-authored-by: Martin Costello <[email protected]>

martincostello reviewed Aug 29, 2025

View reviewed changes

src/OpenTelemetry/CHANGELOG.md Outdated Show resolved Hide resolved

src/Shared/Guard.cs Outdated Show resolved Hide resolved

Update src/OpenTelemetry/CHANGELOG.md

8972d9d

Co-authored-by: Martin Costello <[email protected]>

martincostello approved these changes Aug 29, 2025

View reviewed changes

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 6, 2025

Kielek reviewed Sep 8, 2025

View reviewed changes

github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 9, 2025

improved access for Regex

3933f30

Kielek reviewed Sep 10, 2025

View reviewed changes

Merge remote-tracking branch 'origin' into improve-instrument-name-va…

7895f9e

…lidation-and-log-messages

hannahhaering marked this pull request as draft September 15, 2025 02:59

added metric guard and unit tests

2734a39

hannahhaering marked this pull request as ready for review September 16, 2025 19:22

martincostello approved these changes Sep 18, 2025

View reviewed changes

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 27, 2025

Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Oct 1, 2025

Merge branch 'main' into improve-instrument-name-validation-and-log-m…

ed6a109

…essages

		* Moved the `InstrumentNameRegex` from `MeterProviderBuilderSdk` to `Guard`.
		([#6457](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6457))

Improve instrument name validation and log messages #6457

Are you sure you want to change the base?

Improve instrument name validation and log messages #6457

Uh oh!

Conversation

hannahhaering commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 27, 2025

Uh oh!

Uh oh!

hannahhaering commented Aug 28, 2025 •

edited

Loading

codecov bot commented Sep 15, 2025 •

edited

Loading